You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
• Avoid static context in EnvironmentManager (partially addressed by making properties readonly and improving singleton implementation)
Non-compliant requirements:
• Consolidate test projects (no changes to merge test projects)
• Fix namespace issues (namespace structure remains unchanged)
Requires further human verification:
• Verify if the changes actually improve parallelization capabilities
• Check if the modernized code structure allows for better test execution performance
The EnvironmentManager class still has some fields that were converted to properties but their initialization is not visible in the diff. Ensure all properties are properly initialized.
The method creates a fresh driver but doesn't properly clean up the previous one. This can lead to resource leaks as browser instances may remain open.
public override void AfterTest(ITest test)
{
if (test.Fixture is DriverTestFixture fixtureInstance && this.IsCreatedAfterTest)
{
+ EnvironmentManager.Instance.CloseCurrentDriver();
EnvironmentManager.Instance.CreateFreshDriver();
fixtureInstance.driver = EnvironmentManager.Instance.GetCurrentDriver();
}
base.AfterTest(test);
}
Apply this suggestion
Suggestion importance[1-10]: 9
__
Why: Not closing the previous driver before creating a new one will cause resource leaks as browser instances remain open. This can significantly impact system resources during test runs.
High
Avoid blocking in finalizer
The finalizer is blocking on asynchronous operations which can lead to deadlocks or application hangs. Finalizers should be quick and non-blocking. Consider implementing IDisposable pattern instead.
~EnvironmentManager()
{
- RemoteServer?.StopAsync().Wait();- WebServer?.StopAsync().Wait();+ // Finalizers should not block on async operations
CloseCurrentDriver();
}
Apply this suggestion
Suggestion importance[1-10]: 8
__
Why: Blocking on async operations in finalizers is a serious issue that can lead to deadlocks or application hangs. Finalizers run on a special thread and should complete quickly without blocking operations.
Medium
Use safe casting
Direct casting with (IJavaScriptExecutor) will throw an InvalidCastException if the driver doesn't implement IJavaScriptExecutor. Use the 'as' operator or pattern matching to safely cast.
private static IJavaScriptExecutor GetExecutor(IWebDriver driver)
{
- return (IJavaScriptExecutor)driver;+ return driver as IJavaScriptExecutor ?? throw new ArgumentException("Driver does not support JavaScript execution");
}
Apply this suggestion
Suggestion importance[1-10]: 7
__
Why: Direct casting with parentheses will throw an InvalidCastException if the driver doesn't implement IJavaScriptExecutor. Using the 'as' operator with null check provides safer type conversion with proper error handling.
@nvborisenko What do you think? This modernization should have no behavior changes, and it will allow us to iterate on our testing without adding a million lines which change nothing.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Description
Modernizes some test code and standardizes assembly teardown, in preparation for testing infrastructure improvements.
Motivation and Context
Contributes to #15536
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
Introduced centralized
AssemblyTeardownfor test setup and teardown.AssemblyTeardownclasses for Chrome, Edge, Firefox, IE, Safari, and Remote tests.Refactored
EnvironmentManagerfor improved readability and immutability.EnvironmentManager.Modernized test utilities and attributes for better maintainability.
NeedsFreshDriverAttributeandTestUtilities.Reactivated and updated previously commented-out Firefox tests.
FirefoxProfileManagerTestandFirefoxProfileTests.Changes walkthrough 📝
12 files
Added centralized test setup and teardown for Chrome testsRemoved redundant teardown logic from Chrome testsSimplified property definitions and type checksRefactored driver management logic and removed unused methodsRefactored `EnvironmentManager` for immutability and readabilitySimplified property definitions in `StubDriver`Improved type casting and property handling in utilitiesAdded centralized test setup and teardown for Edge testsAdded centralized test setup and teardown for Firefox testsAdded centralized test setup and teardown for IE testsAdded centralized test setup and teardown for Remote testsAdded centralized test setup and teardown for Safari tests2 files
Reactivated and updated Firefox profile manager testsReactivated and updated Firefox profile tests